-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CUMULUS-3847: Remove any remaining ES indexing functions and tests #3897
Conversation
…loud Metrics usage
packages/api/tests/lambdas/sf-event-sqs-to-db-records/test-write-pdr.js
Outdated
Show resolved
Hide resolved
…d of full removal
…cutions, remove test file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a couple comments. Mostly clarifying questions.
Are there any tests failing/blockers in CI?
Yes, I posted about an aws-client test I'm having issues with in slack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @charleshuang80! This is looking great. I think (pending anything CI uncovers) this should be good to go. I searched the repo and found a couple areas I'm curious about. Notably:
(And other tests that still import es-client) Should those be updated or is that scoped somewhere else?
I think it depends on the test. A few that utilize the cloud metrics functionality still need it I believe. But this test failed in CI and I've been working on removing the ES stuff and trying to figure out if we can still test the performance. I'll do another search, but if you have other specific examples you think should go or just aren't sure about please let me know. |
So I fixed this file and tested it. Also found a few more references that I removed and tested in the latest commit. There are some references in like granules, test-executions, test-build-operation, and test-granules that I think need to stay for Cloud Metrics. If there are any other locations you think I missed please let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @charleshuang80! I think you've got everything that jumps out. If there are areas that still have some ES cruft, we can remove as we find it. The important thing is that the tests pass and all of the core functionality still... functions 😄
…loud Metrics usage
Summary: Summary of changes
Addresses CUMULUS-3847: Remove any remaining ES indexing functions and tests
Changes
PR Checklist